fix(ssh): release port-forward listener on transport death by skevetter · Pull Request #763 · devsy-org/devsy

skevetter · 2026-07-26T15:17:53Z

Refs #759 (secondary bug)

Summary

Local port forwards (portForwarding in pkg/ssh/forward.go) closed their listener only on ctx cancellation or idle timeout — never when the SSH transport died. On a network drop the accept loop stayed blocked on the dead client and the bound host port leaked, so each reconnect failed with bind: address already in use (the reporter saw this once per reconnect on port 7777).

This adds a watcher that cancels the forward context when client.Wait() returns, so the listener is released promptly and portForwarding returns the new ErrTransportClosed, letting the port be rebound on reconnect.

Verification

New regression test TestPortForwarding_ReleasesListenerOnTransportDeath stands up a real SSH client/server, severs the transport, and asserts the forward returns ErrTransportClosed and the host port rebinds.
Verified the test fails without the fix (listener leaked) and passes with it.
Full ./pkg/ssh/... suite (34 tests) green; go vet and full build clean.

Relationship to the keep-alive fix

Separate from #760 (which loosens the server keep-alive so brief stalls don't tear the tunnel down in the first place). This one addresses the port-leak that surfaced on the subsequent reconnect.

Summary by CodeRabbit

Bug Fixes
- SSH port forwarding now reports when the underlying connection closes.
- Forwarding listeners are reliably released after transport failures, allowing the port to be reused.
- Idle timeout and transport-closure errors are now distinguished for clearer handling.

Local port forwards closed their listener only on ctx cancellation or idle timeout, never when the SSH transport died. On a network drop the accept loop stayed blocked on the dead client and the bound host port leaked, so reconnects failed with 'bind: address already in use'. Watch client.Wait() and cancel the forward context when the transport closes, releasing the listener and returning ErrTransportClosed so the port can be rebound on reconnect. Refs #759

netlify · 2026-07-26T15:17:58Z

✅ Deploy Preview for images-devsy-sh canceled.

Name	Link
🔨 Latest commit	`33b4302`
🔍 Latest deploy log	https://app.netlify.com/projects/images-devsy-sh/deploys/6a663736d8afa30008f39e66

coderabbitai · 2026-07-26T15:18:01Z

Warning

Review limit reached

@skevetter, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 49e1f3e6-4459-43b8-9c56-1a23cc70e9a3

📥 Commits

Reviewing files that changed from the base of the PR and between 8504396 and 33b4302.

📒 Files selected for processing (2)

pkg/ssh/forward.go
pkg/ssh/forward_test.go

📝 Walkthrough

Walkthrough

The SSH forwarder now distinguishes transport termination from idle timeout, propagates ErrTransportClosed through its context, and tests that transport failure returns the typed error and releases the local listener.

Changes

SSH transport shutdown

Layer / File(s)	Summary
Transport shutdown propagation `pkg/ssh/forward.go`	Adds `ErrTransportClosed`, monitors `client.Wait()`, cancels forwarding with the typed cause, and returns it from the accept loop.
Transport death cleanup test `pkg/ssh/forward_test.go`	Adds an in-process SSH server and verifies transport termination returns `ErrTransportClosed` and releases the listener.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SSHClient
  participant PortForwarding
  participant LocalListener
  SSHClient->>PortForwarding: Transport closes
  PortForwarding->>PortForwarding: Cancel with ErrTransportClosed
  PortForwarding->>LocalListener: Release listener
  PortForwarding-->>SSHClient: Return ErrTransportClosed

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: fixing SSH port-forward listener cleanup when the transport dies.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

netlify · 2026-07-26T15:18:02Z

✅ Deploy Preview for devsydev canceled.

Name	Link
🔨 Latest commit	`33b4302`
🔍 Latest deploy log	https://app.netlify.com/projects/devsydev/deploys/6a663736935a6400090aa6ec

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/ssh/forward.go (1)
119-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct watcher design; consider logging the transport-close cause.

The watcher correctly converts client.Wait() into a signal that cancels fwdCtx with ErrTransportClosed, and the done-guarded goroutine avoids leaking past normal shutdown. One easy improvement: client.Wait()'s returned error is discarded (_ = client.Wait()), losing the underlying reason the transport died (useful for diagnosing dropped tunnels/reconnects in production).
♻️ Optional: capture and log the Wait() error
 		go func() {
-			_ = client.Wait()
+			if werr := client.Wait(); werr != nil {
+				log.Debugf("ssh transport closed on %s: %v", srcAddr, werr)
+			}
 			close(transportClosed)
 		}()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/ssh/forward.go` around lines 119 - 133, Update the client.Wait watcher in
the client != nil block to capture its returned error and log the
transport-close cause before closing transportClosed, while preserving the
existing done-guarded cancellation flow and ErrTransportClosed behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/ssh/forward_test.go`:
- Around line 111-143: Register cleanup for ln immediately after net.Listen
succeeds, before starting the Accept goroutine or calling ssh.Dial, including
the existing listener-close and server termination cleanup via the test’s
cleanup mechanism. Ensure cleanup runs when Dial fails and preserves the current
cleanup behavior for successful connections.

---

Nitpick comments:
In `@pkg/ssh/forward.go`:
- Around line 119-133: Update the client.Wait watcher in the client != nil block
to capture its returned error and log the transport-close cause before closing
transportClosed, while preserving the existing done-guarded cancellation flow
and ErrTransportClosed behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c209811-34c8-44c5-9506-860e5fe81e7e

📥 Commits

Reviewing files that changed from the base of the PR and between 5311f4e and 8504396.

📒 Files selected for processing (2)

pkg/ssh/forward.go
pkg/ssh/forward_test.go

github-actions Bot added the size/l label Jul 26, 2026

test(ssh): fold transport-death test into forward_test.go, drop comments

051906a

github-actions Bot added size/m and removed size/l labels Jul 26, 2026

test(ssh): pin host key and use t.Context to satisfy gosec/modernize

8504396

skevetter marked this pull request as ready for review July 26, 2026 16:26

coderabbitai Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread pkg/ssh/forward_test.go

fix(ssh): log transport-close cause; close test listener on dial failure

33b4302

skevetter enabled auto-merge (squash) July 26, 2026 17:24

skevetter merged commit 40594c0 into main Jul 26, 2026
65 checks passed

skevetter deleted the fix/portforward-listener-leak branch July 26, 2026 17:36

coderabbitai Bot mentioned this pull request Jul 31, 2026

fix(ssh): wait for reverse-forward listener before running setup-gpg #834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ssh): release port-forward listener on transport death - #763

fix(ssh): release port-forward listener on transport death#763
skevetter merged 4 commits into
mainfrom
fix/portforward-listener-leak

skevetter commented Jul 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

netlify Bot commented Jul 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

netlify Bot commented Jul 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

skevetter commented Jul 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Relationship to the keep-alive fix

Summary by CodeRabbit

Uh oh!

netlify Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for images-devsy-sh canceled.

Uh oh!

coderabbitai Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

netlify Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for devsydev canceled.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

skevetter commented Jul 26, 2026 •

edited by coderabbitai Bot

Loading

netlify Bot commented Jul 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

netlify Bot commented Jul 26, 2026 •

edited

Loading